feat: initial implementation#10
Draft
javier-godoy wants to merge 1 commit into
Draft
Conversation
WalkthroughAdds a new EasyGrid API and implementation: EasyGrid/EasyGridWrapper/EasyGridComposite, EasyColumn and IEasyGridColumn, multi-level ColumnConfiguration (global → instance → column) with renderer factories and renderer helpers; bean-property support, many demos/models/tests, docs, pom and .gitignore updates. ChangesEasyGrid core, column configuration and renderers
Grid API, wrapper and column façade
Demos, models, services
Tests, harness and utilities
Docs, build, metadata
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
src/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.java (1)
49-52: ⚡ Quick winReorganize field declarations before methods.
The
activefield (line 52) is declared after thegetAppointmentTime()method. Java convention is to group all field declarations together before methods. This makes the class structure clearer and easier to navigate.♻️ Suggested reorganization
private String phoneNumber; private LocalDate birthDate; private LocalDateTime appointmentDateTime; private boolean subscriber; + private boolean active; public LocalTime getAppointmentTime() { return appointmentDateTime != null ? appointmentDateTime.toLocalTime() : null; } - private boolean active;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.java` around lines 49 - 52, The field declaration for active is placed after the method getAppointmentTime() in class Person; move the private boolean active; declaration up into the block with the other field declarations (i.e. group all fields together at the top of the class) so that all fields are declared before any methods, preserving existing modifiers and order of other fields.src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.java (1)
33-33: ⚡ Quick winConsider adding
@FunctionalInterfaceannotation.While the interface correctly extends
SerializableFunctionand will be treated as a functional interface by the compiler, explicitly annotating it with@FunctionalInterfaceis a Java best practice that makes the intent clear and enables compile-time verification.📝 Suggested addition
/** * A named functional interface that creates a {`@link` Renderer} for a grid column given a * {`@link` com.vaadin.flow.function.ValueProvider}. * * `@param` <T> the grid bean type * `@param` <V> the column value type */ +@FunctionalInterface public interface RendererFactory<T, V> extends SerializableFunction<ValueProvider<T, V>, Renderer<T>> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.java` at line 33, Add the `@FunctionalInterface` annotation to the RendererFactory interface declaration (RendererFactory<T, V> extends SerializableFunction<ValueProvider<T, V>, Renderer<T>>) so the compiler enforces the functional-interface contract and the intent is explicit; simply annotate the interface line with `@FunctionalInterface` and keep the existing generic signature unchanged.src/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.java (1)
74-80: ⚡ Quick winAvoid relying on Vaadin internal API for descriptor lookup.
Line 77 calls
com.vaadin.flow.internal.BeanUtil, which is explicitly marked "For internal use only. May be renamed or removed in a future release." Use the public JavaBeans API instead:new PropertyDescriptor(propertyName, beanType)directly instantiates the descriptor without requiring Vaadin internals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.java` around lines 74 - 80, The method getPropertyDescriptor in BeanPropertyDefinition currently uses the Vaadin internal BeanUtil (BeanUtil.getPropertyDescriptor) which is unsupported; replace that call with the public JavaBeans API by creating a new PropertyDescriptor(propertyName, beanType) and keep existing exception handling (catch IntrospectionException and rethrow as RuntimeReflectiveOperationException) so behavior remains the same; update imports to use java.beans.PropertyDescriptor and remove dependency on com.vaadin.flow.internal.BeanUtil.src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.java (1)
36-41: 💤 Low valueConsider type-safe renderer factory invocation.
The explicit
<Object>type parameter combined with the cast toLocalDatebypasses compile-time type safety. While this works in the test context, a more type-safe approach would preserve the resolved type.♻️ Alternative approach
- var config = new InstanceEasyGridConfiguration().resolve(LocalDate.class); - var renderer = config.<Object>getRendererFactory().apply(v -> (LocalDate) v); + var config = new InstanceEasyGridConfiguration().resolve(LocalDate.class); + var renderer = config.getRendererFactory().apply(v -> v);The same pattern applies to the other test methods below.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.java` around lines 36 - 41, The test globalLocalDateRendererFactoryIsApplied uses an unchecked cast via config.<Object>getRendererFactory().apply(v -> (LocalDate) v), which bypasses compile-time type safety; change the generic invocation to the resolved type (use config.<LocalDate>getRendererFactory()) and provide a type-safe lambda (e.g., v -> v) so no cast is needed; update the same pattern in the other test methods that call InstanceEasyGridConfiguration.getRendererFactory to preserve type safety and avoid raw/unchecked casts, and verify the result is still instanceOf(LocalDateRenderer.class).src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.java (1)
248-256: 💤 Low valueConsider excluding void.class from the primitive sortability check.
Line 252 uses
type.isPrimitive()to determine sortability, butvoid.class.isPrimitive()returnstrueeven though void values cannot be compared. While this is unlikely to occur in practice (since column value providers wouldn't return void), adding an explicit check would make the code more defensive.🛡️ Optional defensive check
- if (Comparable.class.isAssignableFrom(type) || type.isPrimitive()) { + if (Comparable.class.isAssignableFrom(type) || (type.isPrimitive() && type != void.class)) { column.setComparator((ValueProvider) getter); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.java` around lines 248 - 256, In createEasyColumn(Class<V> type, ValueProvider<T, V> getter) the sortability check currently treats all primitives as sortable; make it defensive by excluding void.class from the primitive branch so void values aren't considered comparable — change the condition that sets column.setComparator(...) (referencing createEasyColumn, type.isPrimitive(), and Comparable.class.isAssignableFrom) to only treat primitives as sortable when type != void.class.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONFIGURATION_RESOLUTION.md`:
- Around line 13-20: Several fenced code blocks in CONFIGURATION_RESOLUTION.md
are unlabeled and trigger MD040; update each triple-backtick fence that contains
sequences like "I·Foo → I·Entity → G·Foo → G·Entity → Default", "I·Foo → G·Foo →
I·Entity → G·Entity → Default", "per-column → I·Foo → I·Entity → I·Object →
G·Foo → G·Entity → G·Object → Default", "I·Foo(impl) → I·Entity(impl) →
I·Object(impl)", "G·Foo(impl) → G·Entity(impl) → G·Object(impl)", and
"ColumnConfigurationLink(primary = I·Foo chain, fallback = G·Foo chain)" to use
a language identifier (e.g., text) after the opening ``` so the fences read
```text and satisfy markdown linting.
In `@FEATURE_ROW_ACTIONS.md`:
- Around line 51-54: The example uses easyGrid.addRowAction but refreshes using
grid.getDataProvider().refreshAll(), causing a variable-name mismatch; update
the refresh call to use easyGrid.getDataProvider().refreshAll() (or consistently
rename the grid variable so addRowAction, the deletion call
personService.delete(person), and the subsequent refresh all reference the same
grid instance) so the snippet is internally consistent.
In `@SPECIFICATIONS.md`:
- Around line 184-189: The fenced code block containing the snippet that starts
with "Column·Foo" is missing a language tag; update the opening fence from ```
to ```text so the block is explicitly marked (e.g., change the fence before
"Column·Foo" to "```text") to satisfy MD040; keep the block contents unchanged
and leave the closing fence as ``` to complete the fix.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.java`:
- Around line 66-69: The catch block in BeanPropertyDefinition that currently
catches NoSuchElementException | IllegalArgumentException and rethrows a new
IllegalArgumentException for "Can't resolve property name '" + propertyName + "'
from '" + propertySet + "'" must preserve the original exception as the cause;
modify the throw in that catch to pass the caught exception (e) as the cause
when constructing the new IllegalArgumentException so the root cause is retained
in stack traces and logs, keeping the same message but using the constructor
IllegalArgumentException(String, Throwable) (or equivalent) referencing
propertyName and propertySet.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationLink.java`:
- Around line 38-41: The class ColumnConfigurationLink currently allows a null
primary which causes setters that call primary.* to NPE at runtime; ensure
primary is non-null by adding an explicit null check (e.g.
Objects.requireNonNull(primary, "primary must not be null")) in the
ColumnConfigurationLink constructor(s) and update any factory methods to enforce
the same, and keep fallback nullable as before; this guarantees setters that
call primary.* (and any methods in ColumnConfigurationLink that assume primary
!= null) are safe.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.java`:
- Around line 70-87: There is a race between freeze() and forType(Class<V>)
because the check of the static boolean frozen and the call to
map.getOrCreate(type) are not atomic; fix it by synchronizing access: make
freeze() and forType(...) coordinate on the same lock (e.g., synchronize on
GlobalEasyGridConfiguration.class or add a private static final Object LOCK) and
perform the frozen check and the call to map.getOrCreate(type) inside the same
synchronized block (or use a ReentrantReadWriteLock with write lock in freeze()
and read lock in forType()) so no concurrent thread can create a registration
after freeze() completes.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.java`:
- Around line 77-79: The Boolean formatter on
grid.getColumn("subscriber").as(Boolean.class).setFormatter(...) is not
null-safe; update the formatter to guard against null (e.g., treat null as false
or return an explicit placeholder like "Unknown") before dereferencing so it
never throws NPE. Modify the lambda passed to setFormatter on the subscriber
column to first check v for null and then return "Subscribed" or "Not
Subscribed" (or your chosen null placeholder) accordingly.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.java`:
- Around line 36-43: The constructor NumberSample(BigDecimal) currently converts
bigDecimal to smaller integer types without overflow checks (fields: bigDecimal,
bigInteger, integer, intValue, longValue, doubleValue), causing silent overflow
for values like 1234567890123.456; either: 1) update the conversions to use
exact checks (use bigInteger = value.toBigInteger(); integer =
bigInteger.intValueExact(); intValue = integer; longValue =
bigInteger.longValueExact();) so an ArithmeticException is thrown on overflow,
or 2) change the demo input in NumberRenderingDemo to a smaller BigDecimal that
fits int/long ranges, or add a comment documenting that overflow is intentional
— pick one approach and apply it consistently to NumberSample's constructor and
the demo.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java`:
- Around line 30-37: The fetch method should validate and clamp pagination
bounds before calling personData.getPersons().subList: ensure offset and limit
are non-negative (treat negative as 0), if limit == 0 return empty list
immediately, clamp offset to be at most size (if offset >= size return empty
list), compute end = min(offset + limit, size) and ensure end >= offset before
calling subList; update the fetch(...) implementation to perform these guards
using the existing fetch method and personData.getPersons() references.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.java`:
- Around line 126-133: The buildArg method in DelegationTest only handles
boolean and int primitives and will fail for other primitive parameter types;
update buildArg to cover all primitive types (long, short, byte, char, float,
double) by adding branches like long.class -> Long.valueOf(0L), double.class ->
Double.valueOf(0.0), float.class -> Float.valueOf(0.0f), short.class ->
Short.valueOf((short)0), byte.class -> Byte.valueOf((byte)0), char.class ->
Character.valueOf('\0') while keeping the existing boolean and int branches, and
preserve the existing array and Mockito.mock(type) fallback so arrays and
reference types still behave as before.
- Around line 188-190: The isChainable method is using the wrong variable and a
brittle name-based heuristic; change it to inspect the method's return type
using the targetMethod parameter and treat methods returning void as
non-chainable (e.g., return !targetMethod.getReturnType().equals(Void.TYPE));
update references from method to targetMethod in isChainable to fix the variable
mismatch and derive chainability from the return type instead.
In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java`:
- Around line 68-73: The computed-days-until-birthday code in
TypedColumnDemo.java can throw for Feb 29 births when calling
p.getBirthDate().withYear(today.getYear()); fix by computing the birthday via
MonthDay (e.g., MonthDay birthMd = MonthDay.from(p.getBirthDate())), then create
birthday = birthMd.isValidYear(today.getYear()) ?
birthMd.atYear(today.getYear()) : MonthDay.of(2,28).atYear(today.getYear()) (or
another chosen fallback) so Feb 29 is handled safely, then keep the existing
logic that pushes the birthday to next year and returns today.until(birthday,
ChronoUnit.DAYS).
---
Nitpick comments:
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.java`:
- Around line 74-80: The method getPropertyDescriptor in BeanPropertyDefinition
currently uses the Vaadin internal BeanUtil (BeanUtil.getPropertyDescriptor)
which is unsupported; replace that call with the public JavaBeans API by
creating a new PropertyDescriptor(propertyName, beanType) and keep existing
exception handling (catch IntrospectionException and rethrow as
RuntimeReflectiveOperationException) so behavior remains the same; update
imports to use java.beans.PropertyDescriptor and remove dependency on
com.vaadin.flow.internal.BeanUtil.
In `@src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.java`:
- Around line 248-256: In createEasyColumn(Class<V> type, ValueProvider<T, V>
getter) the sortability check currently treats all primitives as sortable; make
it defensive by excluding void.class from the primitive branch so void values
aren't considered comparable — change the condition that sets
column.setComparator(...) (referencing createEasyColumn, type.isPrimitive(), and
Comparable.class.isAssignableFrom) to only treat primitives as sortable when
type != void.class.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.java`:
- Line 33: Add the `@FunctionalInterface` annotation to the RendererFactory
interface declaration (RendererFactory<T, V> extends
SerializableFunction<ValueProvider<T, V>, Renderer<T>>) so the compiler enforces
the functional-interface contract and the intent is explicit; simply annotate
the interface line with `@FunctionalInterface` and keep the existing generic
signature unchanged.
In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.java`:
- Around line 49-52: The field declaration for active is placed after the method
getAppointmentTime() in class Person; move the private boolean active;
declaration up into the block with the other field declarations (i.e. group all
fields together at the top of the class) so that all fields are declared before
any methods, preserving existing modifiers and order of other fields.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.java`:
- Around line 36-41: The test globalLocalDateRendererFactoryIsApplied uses an
unchecked cast via config.<Object>getRendererFactory().apply(v -> (LocalDate)
v), which bypasses compile-time type safety; change the generic invocation to
the resolved type (use config.<LocalDate>getRendererFactory()) and provide a
type-safe lambda (e.g., v -> v) so no cast is needed; update the same pattern in
the other test methods that call
InstanceEasyGridConfiguration.getRendererFactory to preserve type safety and
avoid raw/unchecked casts, and verify the result is still
instanceOf(LocalDateRenderer.class).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a325efc3-43a7-4ca9-b6f0-4641a95438d9
📒 Files selected for processing (56)
.gitignoreCONFIGURATION_RESOLUTION.mdFEATURE_ROW_ACTIONS.mdREADME.mdSPECIFICATIONS.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGrid.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/RuntimeReflectiveOperationException.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationImpl.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationLink.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationTextRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/EasyGridConfigurationClassMap.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/InstanceEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/NumberRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.javasrc/test/java/com/flowingcode/vaadin/addons/DemoLayout.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/AutoColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ConfigurationHierarchyDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/DemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/GridColumnMethodLister.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/NumberRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/SelectiveColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypeRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/data/PersonData.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Address.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationImplParentDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationLinkDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnTestHelper.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToConfigurationDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToGridColumnDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/SerializationTest.javasrc/test/resources/META-INF/frontend/styles/easy-grid-demo-styles.csssrc/test/resources/META-INF/frontend/styles/shared-styles.css
💤 Files with no reviewable changes (2)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.java
Comment on lines
+13
to
+20
| ``` | ||
| I·Foo → I·Entity → G·Foo → G·Entity → Default | ||
| ``` | ||
|
|
||
| **Type-first** exhausts all Foo-specific settings before looking at Entity: | ||
| ``` | ||
| I·Foo → G·Foo → I·Entity → G·Entity → Default | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
Line 13, Line 18, Line 78, Line 86, Line 92, and Line 98 use unlabeled fences, which triggers MD040 and can fail markdown lint gates.
Suggested fix
-```
+```text
I·Foo → I·Entity → G·Foo → G·Entity → Default- +text
I·Foo → G·Foo → I·Entity → G·Entity → Default
-```
+```text
per-column → I·Foo → I·Entity → I·Object → G·Foo → G·Entity → G·Object → Default
- +text
I·Foo(impl) → I·Entity(impl) → I·Object(impl)
-```
+```text
G·Foo(impl) → G·Entity(impl) → G·Object(impl)
- +text
ColumnConfigurationLink(primary = I·Foo chain, fallback = G·Foo chain)
Also applies to: 78-100
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONFIGURATION_RESOLUTION.md` around lines 13 - 20, Several fenced code blocks
in CONFIGURATION_RESOLUTION.md are unlabeled and trigger MD040; update each
triple-backtick fence that contains sequences like "I·Foo → I·Entity → G·Foo →
G·Entity → Default", "I·Foo → G·Foo → I·Entity → G·Entity → Default",
"per-column → I·Foo → I·Entity → I·Object → G·Foo → G·Entity → G·Object →
Default", "I·Foo(impl) → I·Entity(impl) → I·Object(impl)", "G·Foo(impl) →
G·Entity(impl) → G·Object(impl)", and "ColumnConfigurationLink(primary = I·Foo
chain, fallback = G·Foo chain)" to use a language identifier (e.g., text) after
the opening ``` so the fences read ```text and satisfy markdown linting.
Member
Author
There was a problem hiding this comment.
Defer to reviewers' judgment. This file is intended for AI consumption.
There was a problem hiding this comment.
@javier-godoy, understood — I'll dismiss this concern. Since CONFIGURATION_RESOLUTION.md is intended for AI consumption rather than human readers or standard documentation pipelines, markdown lint compliance is not a meaningful concern here.
✏️ Learnings added
Learnt from: javier-godoy
Repo: FlowingCode/EasyGridAddon PR: 10
File: CONFIGURATION_RESOLUTION.md:13-20
Timestamp: 2026-05-14T12:05:24.375Z
Learning: The file `CONFIGURATION_RESOLUTION.md` in the `FlowingCode/EasyGridAddon` repository is intended for AI consumption, not for human readers or standard documentation pipelines. Markdown style/lint issues (e.g., MD040 unlabeled fenced code blocks) should not be flagged in this file during code reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java (1)
67-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle leap-day birthdays safely in computed column logic.
Line 69/Line 72 can throw at runtime for Feb 29 birthdays in non-leap years due to
withYear(...), which can break rendering for those rows.Suggested fix
import java.time.LocalDate; +import java.time.MonthDay; import java.time.temporal.ChronoUnit; @@ grid.addColumn(Integer.class, p -> { LocalDate today = LocalDate.now(); - LocalDate birthday = p.getBirthDate().withYear(today.getYear()); + MonthDay birthMd = MonthDay.from(p.getBirthDate()); + LocalDate birthday = birthMd.atYear(today.getYear()); if (birthday.isBefore(today)) { - birthday = p.getBirthDate().withYear(today.getYear() + 1); + birthday = birthMd.atYear(today.getYear() + 1); } return (int) ChronoUnit.DAYS.between(today, birthday); }).setHeader("Days Until Birthday");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java` around lines 67 - 73, The computed-birthday logic in grid.addColumn (the lambda using p.getBirthDate() and withYear(...)) can throw for Feb 29 birthdays in non-leap years; update the code that computes birthday to choose a valid day for the target year instead of blindly calling withYear. Concretely, compute targetYear = today.getYear() (and targetYear+1 where used), then build birthday using LocalDate.of(targetYear, birthDate.getMonthValue(), Math.min(birthDate.getDayOfMonth(), Year.of(targetYear).length())) (or wrap withYear in a try/catch and fallback to the last valid day of the month) so Feb 29s are mapped safely to Feb 28 in non-leap years before the before/after comparisons.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java`:
- Around line 67-73: The computed-birthday logic in grid.addColumn (the lambda
using p.getBirthDate() and withYear(...)) can throw for Feb 29 birthdays in
non-leap years; update the code that computes birthday to choose a valid day for
the target year instead of blindly calling withYear. Concretely, compute
targetYear = today.getYear() (and targetYear+1 where used), then build birthday
using LocalDate.of(targetYear, birthDate.getMonthValue(),
Math.min(birthDate.getDayOfMonth(), Year.of(targetYear).length())) (or wrap
withYear in a try/catch and fallback to the last valid day of the month) so Feb
29s are mapped safely to Feb 28 in non-leap years before the before/after
comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b032e9f-8d3a-46cd-983c-6557e27a2096
📒 Files selected for processing (11)
pom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.java
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.java
- pom.xml
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.java (1)
51-54: 💤 Low valueConsider clarifying the test name.
The test name suggests it verifies that an entry is created, but the implementation only checks that
resolve()returns non-null without verifying whether a new entry was actually created. Since LocalDate is pre-registered (per the comment on line 71), this test may be verifying retrieval rather than creation. Consider renaming to something likeresolveBeforeFreezeReturnsNonNullor adding assertions to explicitly verify creation behavior if that's the intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.java` around lines 51 - 54, The test name resolveBeforeFreezeCreatesEntry is misleading because it only asserts GlobalEasyGridConfiguration.resolve(LocalDate.class) != null; either rename the test to resolveBeforeFreezeReturnsNonNull (or similar) to reflect its behavior, or modify the test to explicitly verify creation (e.g., assert absence in the registry before calling resolve and presence after) by using the registry accessors used elsewhere in GlobalEasyGridConfiguration; update the method name and any references accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.java`:
- Around line 55-59: The test currently uses hasItems(...) which allows extra
unexpected columns; update the assertion in EasyGridConstructionTest so the
collected keys (from grid.getWrappedGrid().getColumns() -> keys) are asserted
for exact content/size — e.g., replace the hasItems assertion with an
exact-match assertion (such as assertThat(keys, contains(...)) or assertEquals
on a List of expected keys) to ensure no extra columns are present.
---
Nitpick comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.java`:
- Around line 51-54: The test name resolveBeforeFreezeCreatesEntry is misleading
because it only asserts GlobalEasyGridConfiguration.resolve(LocalDate.class) !=
null; either rename the test to resolveBeforeFreezeReturnsNonNull (or similar)
to reflect its behavior, or modify the test to explicitly verify creation (e.g.,
assert absence in the registry before calling resolve and presence after) by
using the registry accessors used elsewhere in GlobalEasyGridConfiguration;
update the method name and any references accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17eb997e-af69-427a-8309-02c50e552807
📒 Files selected for processing (3)
src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java
Comment on lines
+55
to
+59
| List<String> keys = grid.getWrappedGrid().getColumns().stream() | ||
| .map(Grid.Column::getKey) | ||
| .collect(Collectors.toList()); | ||
| assertThat(keys, hasItems("firstName", "lastName", "age", "birthDate", "subscriber", "active")); | ||
| } |
There was a problem hiding this comment.
Tighten the auto-discovery assertion to catch unexpected columns.
hasItems(...) alone passes even if extra columns are created. Add an exact-size (or exact-content) assertion so this test fails on unintended auto-discovery changes.
Suggested patch
List<String> keys = grid.getWrappedGrid().getColumns().stream()
.map(Grid.Column::getKey)
.collect(Collectors.toList());
+ assertThat(keys, hasSize(6));
assertThat(keys, hasItems("firstName", "lastName", "age", "birthDate", "subscriber", "active"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| List<String> keys = grid.getWrappedGrid().getColumns().stream() | |
| .map(Grid.Column::getKey) | |
| .collect(Collectors.toList()); | |
| assertThat(keys, hasItems("firstName", "lastName", "age", "birthDate", "subscriber", "active")); | |
| } | |
| List<String> keys = grid.getWrappedGrid().getColumns().stream() | |
| .map(Grid.Column::getKey) | |
| .collect(Collectors.toList()); | |
| assertThat(keys, hasSize(6)); | |
| assertThat(keys, hasItems("firstName", "lastName", "age", "birthDate", "subscriber", "active")); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.java`
around lines 55 - 59, The test currently uses hasItems(...) which allows extra
unexpected columns; update the assertion in EasyGridConstructionTest so the
collected keys (from grid.getWrappedGrid().getColumns() -> keys) are asserted
for exact content/size — e.g., replace the hasItems assertion with an
exact-match assertion (such as assertThat(keys, contains(...)) or assertEquals
on a List of expected keys) to ensure no extra columns are present.
Member
Author
There was a problem hiding this comment.
The reviewer is wrong for two reasons.\n\n1. The test is intentionally scoped. The test name is autoDiscoveryCreatesColumnsForBeanProperties — it asserts that auto-discovery does pick up the expected properties. It makes no claim about the complete set of columns. Person has more properties than those six (e.g., phoneNumber, appointmentDateTime) that auto-discovery will also create columns for. Using contains(...) or asserting exact size would couple this test to the full property set of Person, making it break every time Person gets a new field — even if auto-discovery is working correctly.\n\n2. The complementary exact-match test already exists. explicitPropertyConstructorCreatesOnlyListedColumns uses both hasSize(2) and hasItems(...) to assert exact column membership. That is the test whose contract is "only these columns, nothing else." The division of concerns is deliberate: one test verifies auto-discovery picks up expected properties; the other verifies the explicit constructor creates exactly the specified columns.\n\nRequiring exact-match in autoDiscoveryCreatesColumnsForBeanProperties would duplicate the concern of explicitPropertyConstructorCreatesOnlyListedColumns while also making auto-discovery tests unnecessarily brittle to model changes.
Member
Author
There was a problem hiding this comment.
The reviewer is wrong for two reasons.\n\n1. The test is intentionally scoped. The test name is autoDiscoveryCreatesColumnsForBeanProperties — it asserts that auto-discovery does pick up the expected properties. It makes no claim about the complete set of columns. Person has more properties than those six (e.g., phoneNumber, appointmentDateTime) that auto-discovery will also create columns for. Using contains(...) or asserting exact size would couple this test to the full property set of Person, making it break every time Person gets a new field — even if auto-discovery is working correctly.\n\n2. The complementary exact-match test already exists. explicitPropertyConstructorCreatesOnlyListedColumns uses both hasSize(2) and hasItems(...) to assert exact column membership. That is the test whose contract is "only these columns, nothing else." The division of concerns is deliberate: one test verifies auto-discovery picks up expected properties; the other verifies the explicit constructor creates exactly the specified columns.\n\nRequiring exact-match in autoDiscoveryCreatesColumnsForBeanProperties would duplicate the concern of explicitPropertyConstructorCreatesOnlyListedColumns while also making auto-discovery tests unnecessarily brittle to model changes.
There was a problem hiding this comment.
@javier-godoy, you're absolutely right on both counts — I apologize for the noise. The split is clear and well-reasoned: autoDiscoveryCreatesColumnsForBeanProperties is a scoped "subset" assertion that survives Person model evolution, while explicitPropertyConstructorCreatesOnlyListedColumns carries the exact-membership contract. Adding hasSize to the auto-discovery test would couple it to the full property set and contradict its stated intent.
(\_/)
(o . o)
(> 🥕 <)
✏️ Learnings added
Learnt from: javier-godoy
Repo: FlowingCode/EasyGridAddon PR: 10
File: src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.java:55-59
Timestamp: 2026-05-14T14:36:17.429Z
Learning: In EasyGridConstructionTest, `autoDiscoveryCreatesColumnsForBeanProperties` intentionally uses `hasItems(...)` without an exact-size assertion. The test only verifies that auto-discovery picks up a known subset of expected properties; `Person` has additional auto-discovered properties (e.g., `phoneNumber`, `appointmentDateTime`) beyond those six. The exact-membership contract ("only these columns, nothing else") belongs to `explicitPropertyConstructorCreatesOnlyListedColumns`, which uses both `hasSize` and `hasItems`. Do not flag the auto-discovery test for lacking a size assertion — the division of concerns is deliberate to avoid coupling the test to the full `Person` property set.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: javier-godoy
Repo: FlowingCode/EasyGridAddon PR: 10
File: src/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.java:77-79
Timestamp: 2026-05-14T11:59:45.173Z
Learning: When reviewing EasyGridAddon code that registers a formatter via `EasyColumn.setFormatter(...)` (i.e., the lambda parameter `val`), assume `val` is non-null. `ColumnConfigurationTextRenderer` guards `formatter.apply(val)` with a null check and returns `config.getNullRepresentation()` for `val == null`, so formatter lambdas registered through `setFormatter` should not be flagged as “null-unsafe” for dereferencing `val` without an explicit null check. Only flag null-safety issues if the formatter is called from a path that does not follow this guard.
There was a problem hiding this comment.
@javier-godoy, you're absolutely right — I apologize for the incorrect suggestion. The two-point reasoning is sound:
autoDiscoveryCreatesColumnsForBeanPropertiesuseshasItemsdeliberately becausePersonhas additional properties beyond those six, and the test's contract is only "these properties are discovered," not "only these properties exist."- The exact-membership contract is already covered by
explicitPropertyConstructorCreatesOnlyListedColumnswithhasSize+hasItems, making an exact assertion here redundant and brittleness-inducing.
The division of concerns is well-designed.
(ᵔ.ᵔ) ~ noted
✏️ Learnings added
Learnt from: javier-godoy
Repo: FlowingCode/EasyGridAddon PR: 10
File: src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.java:55-59
Timestamp: 2026-05-14T14:36:24.110Z
Learning: In `EasyGridConstructionTest` (`src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.java`), the test `autoDiscoveryCreatesColumnsForBeanProperties` intentionally uses `hasItems(...)` without an exact-size assertion because `Person` has more properties than those asserted (e.g., `phoneNumber`, `appointmentDateTime` also get auto-discovered columns). The test only verifies that the expected subset of properties is discovered. The exact-membership contract ("only these columns, nothing else") is handled by the separate test `explicitPropertyConstructorCreatesOnlyListedColumns`, which uses both `hasSize` and `hasItems`. Do not flag `autoDiscoveryCreatesColumnsForBeanProperties` for lacking an exact-size assertion — that is intentional by design.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: javier-godoy
Repo: FlowingCode/EasyGridAddon PR: 10
File: src/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.java:77-79
Timestamp: 2026-05-14T11:59:45.173Z
Learning: When reviewing EasyGridAddon code that registers a formatter via `EasyColumn.setFormatter(...)` (i.e., the lambda parameter `val`), assume `val` is non-null. `ColumnConfigurationTextRenderer` guards `formatter.apply(val)` with a null check and returns `config.getNullRepresentation()` for `val == null`, so formatter lambdas registered through `setFormatter` should not be flagged as “null-unsafe” for dereferencing `val` without an explicit null check. Only flag null-safety issues if the formatter is called from a path that does not follow this guard.
javier-godoy
force-pushed
the
first-iteration
branch
from
8002919 to
23b26a3
Compare
Member
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java (1)
30-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate pagination inputs before
skip/limit.
offset < 0orlimit < 0will fail at runtime. Guard and clamp bounds before slicing.Suggested fix
public List<Person> fetch(int offset, int limit) { - return personData.getPersons().stream().skip(offset).limit(limit).toList(); + if (offset < 0 || limit < 0) { + throw new IllegalArgumentException("offset and limit must be non-negative"); + } + List<Person> persons = personData.getPersons(); + int size = persons.size(); + if (offset >= size || limit == 0) { + return List.of(); + } + int end = (int) Math.min((long) size, (long) offset + limit); + return persons.subList(offset, end); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java` around lines 30 - 31, The fetch method may throw for negative pagination values; in PersonService.fetch validate and clamp offset/limit before slicing the list from personData.getPersons(): compute safeOffset = max(0, offset) and safeLimit = max(0, limit), get the list size, return empty list if safeOffset >= size, otherwise call stream().skip(safeOffset).limit(min(safeLimit, size - safeOffset)).toList() so skip/limit never receive negative or out-of-range values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.java`:
- Around line 104-106: resolve(...) returns the actual mutable
ColumnConfiguration instance when frozen, so callers can still mutate global
state; change resolve to return a defensive copy or an immutable/read-only view
when frozen (e.g., if frozen return a copy or an unmodifiable wrapper of
map.get(type) instead of the live instance). Locate the resolve method and
replace the frozen branch to return a safe copy (or wrap via a new
ReadOnlyColumnConfiguration(map.get(type)) or map.get(type).copy() / clone()),
ensuring nulls are handled the same as forType(type).
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnTestHelper.java`:
- Around line 45-46: The mocked renderer factory ignores its input and always
renders item.toString(); update the Mockito stub for mock.getRendererFactory()
in EasyColumnTestHelper to use the provided getter parameter: return a
TextRenderer that calls getter.apply(item) (or getter.apply(item)::toString as
appropriate) so the renderer uses the actual value-provider passed into the
factory instead of Object::toString; keep using
com.vaadin.flow.data.renderer.TextRenderer and ensure the lambda references the
factory parameter named getter.
In `@src/test/resources/META-INF/frontend/styles/shared-styles.css`:
- Around line 1-20: The CSS comment blocks in shared-styles.css violate the
comment-whitespace-inside rule; fix by ensuring all block comments use a single
space after the opening /* and a single space before the closing */, e.g. change
/*- and /*Demo styles*/ to /* - ... */ and /* Demo styles */ (apply consistently
across the header license block and any short comments) so stylelint passes.
---
Duplicate comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java`:
- Around line 30-31: The fetch method may throw for negative pagination values;
in PersonService.fetch validate and clamp offset/limit before slicing the list
from personData.getPersons(): compute safeOffset = max(0, offset) and safeLimit
= max(0, limit), get the list size, return empty list if safeOffset >= size,
otherwise call stream().skip(safeOffset).limit(min(safeLimit, size -
safeOffset)).toList() so skip/limit never receive negative or out-of-range
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25cf5000-9af9-424b-8b60-6f55ca439bbe
📒 Files selected for processing (58)
.gitignoreCONFIGURATION_RESOLUTION.mdFEATURE_ROW_ACTIONS.mdREADME.mdSPECIFICATIONS.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGrid.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/RuntimeReflectiveOperationException.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationImpl.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationLink.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationTextRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/EasyGridConfigurationClassMap.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/InstanceEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/NumberRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.javasrc/test/java/com/flowingcode/vaadin/addons/DemoLayout.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/AutoColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ConfigurationHierarchyDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/DemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/GridColumnMethodLister.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/NumberRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/SelectiveColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypeRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/data/PersonData.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Address.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationImplParentDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationLinkDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnTestHelper.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToConfigurationDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToGridColumnDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/SerializationTest.javasrc/test/resources/META-INF/frontend/styles/easy-grid-demo-styles.csssrc/test/resources/META-INF/frontend/styles/shared-styles.css
💤 Files with no reviewable changes (2)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.java
Comment on lines
+104
to
+106
| public <V> ColumnConfiguration<V> resolve(Class<V> type) { | ||
| return frozen ? map.get(type) : forType(type); | ||
| } |
There was a problem hiding this comment.
resolve() currently allows post-freeze global mutation.
After freeze(), callers can still do GlobalEasyGridConfiguration.resolve(type).set..., which mutates global config and bypasses the “locked after startup” contract.
🔧 Minimal fix
- public <V> ColumnConfiguration<V> resolve(Class<V> type) {
+ static <V> ColumnConfiguration<V> resolve(Class<V> type) {
return frozen ? map.get(type) : forType(type);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public <V> ColumnConfiguration<V> resolve(Class<V> type) { | |
| return frozen ? map.get(type) : forType(type); | |
| } | |
| static <V> ColumnConfiguration<V> resolve(Class<V> type) { | |
| return frozen ? map.get(type) : forType(type); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.java`
around lines 104 - 106, resolve(...) returns the actual mutable
ColumnConfiguration instance when frozen, so callers can still mutate global
state; change resolve to return a defensive copy or an immutable/read-only view
when frozen (e.g., if frozen return a copy or an unmodifiable wrapper of
map.get(type) instead of the live instance). Locate the resolve method and
replace the frozen branch to return a safe copy (or wrap via a new
ReadOnlyColumnConfiguration(map.get(type)) or map.get(type).copy() / clone()),
ensuring nulls are handled the same as forType(type).
javier-godoy
force-pushed
the
first-iteration
branch
from
23b26a3 to
0691f32
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONFIGURATION_RESOLUTION.md`:
- Around line 31-33: Update the docs to use the canonical API name: replace
examples calling easyGrid.configuration.forType(...) with the spec-defined
easyGrid.typeConfiguration(Class) usage; ensure all occurrences (including the
other example block currently showing easyGrid.configuration.forType) are
changed so the docs consistently reference easyGrid.typeConfiguration and its
Class-based signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd8a906a-8d1b-4d18-b379-b191c49380a3
📒 Files selected for processing (58)
.gitignoreCONFIGURATION_RESOLUTION.mdFEATURE_ROW_ACTIONS.mdREADME.mdSPECIFICATIONS.mdpom.xmlsrc/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGrid.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/RuntimeReflectiveOperationException.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationImpl.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationLink.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationTextRenderer.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/EasyGridConfigurationClassMap.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/config/InstanceEasyGridConfiguration.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalTimeRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/NumberRenderers.javasrc/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.javasrc/test/java/com/flowingcode/vaadin/addons/DemoLayout.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/AutoColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/ConfigurationHierarchyDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/DemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/GridColumnMethodLister.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/NumberRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/SelectiveColumnsDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypeRenderingDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/data/PersonData.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Address.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationImplParentDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationLinkDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnTestHelper.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToConfigurationDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToGridColumnDelegationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.javasrc/test/java/com/flowingcode/vaadin/addons/easygrid/test/SerializationTest.javasrc/test/resources/META-INF/frontend/styles/easy-grid-demo-styles.csssrc/test/resources/META-INF/frontend/styles/shared-styles.css
💤 Files with no reviewable changes (3)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemo.java
- src/test/resources/META-INF/frontend/styles/shared-styles.css
- src/test/java/com/flowingcode/vaadin/addons/easygrid/it/ViewIT.java
✅ Files skipped from review due to trivial changes (8)
- .gitignore
- src/main/java/com/flowingcode/vaadin/addons/easygrid/RuntimeReflectiveOperationException.java
- src/test/resources/META-INF/frontend/styles/easy-grid-demo-styles.css
- README.md
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/InstanceEasyGridConfiguration.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/it/AbstractViewTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/DemoView.java
- src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java
🚧 Files skipped from review as they are similar to previous changes (44)
- src/test/java/com/flowingcode/vaadin/addons/easygrid/EasyGridDemoView.java
- FEATURE_ROW_ACTIONS.md
- src/test/java/com/flowingcode/vaadin/addons/easygrid/model/Address.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/model/NumberSample.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/GridColumnMethodLister.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/model/Person.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/EasyGridConfigurationClassMap.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateTimeRenderers.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationTextRenderer.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/RendererFactory.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToGridColumnDelegationTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/NumberRenderingDemo.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalEasyGridConfigurationFreezeTest.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGrid.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyGridConstructionTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/TypedColumnDemo.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/SerializationTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/SelectiveColumnsDemo.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationLink.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfiguration.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/NumberRenderers.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/ConfigurationHierarchyDemo.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridColumn.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnTestHelper.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/service/PersonService.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalDateRenderers.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/TypeRenderingDemo.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/GlobalRendererFactoryTest.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridComposite.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/data/PersonData.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/ColumnConfigurationImpl.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyGridWrapper.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/BeanPropertyDefinition.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/EasyColumn.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/EasyColumnToConfigurationDelegationTest.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/renderers/LocalTimeRenderers.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationImplParentDelegationTest.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/ColumnConfigurationLinkDelegationTest.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/config/GlobalEasyGridConfiguration.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/ColumnConfigurationDemo.java
- src/main/java/com/flowingcode/vaadin/addons/easygrid/IEasyGridComposite.java
- src/test/java/com/flowingcode/vaadin/addons/easygrid/test/DelegationTest.java
- pom.xml
Comment on lines
+31
to
+33
| ```java | ||
| easyGrid.configuration.forType(Entity.class).setTextAlign(RIGHT); | ||
| ``` |
There was a problem hiding this comment.
Use the same instance-configuration API name as the spec (typeConfiguration).
These examples use easyGrid.configuration.forType(...), but the spec defines instance type config via easyGrid.typeConfiguration(Class). Keeping one canonical API name avoids ambiguity in generated usage guidance.
Suggested doc fix
- easyGrid.configuration.forType(Entity.class).setTextAlign(RIGHT);
+ easyGrid.typeConfiguration(Entity.class).setTextAlign(RIGHT);
- easyGrid.configuration.forType(Entity.class).setFormatter(e -> e.getId().toString());
+ easyGrid.typeConfiguration(Entity.class).setFormatter(e -> e.getId().toString());Also applies to: 50-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONFIGURATION_RESOLUTION.md` around lines 31 - 33, Update the docs to use the
canonical API name: replace examples calling easyGrid.configuration.forType(...)
with the spec-defined easyGrid.typeConfiguration(Class) usage; ensure all
occurrences (including the other example block currently showing
easyGrid.configuration.forType) are changed so the docs consistently reference
easyGrid.typeConfiguration and its Class-based signature.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit